GH-50654: [C++] Support simdjson without exceptions - #50672
Conversation
f94fe05 to
fde573a
Compare
fde573a to
92ae650
Compare
| std::string_view str; | ||
| if (auto error = std::move(str_result).get(str)) { | ||
| return Status::Invalid("Error getting string for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } | ||
| return std::string(str); |
There was a problem hiding this comment.
#50653 adds
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
that already returns arrow::Results instead of error codes. This might simplify these type-casts
There was a problem hiding this comment.
I wanted to write after this PR is merged. Referenced the wrong PR. Fixed it.
| @@ -102,7 +106,13 @@ class ObjectParser::Impl { | |||
| "': (code=", static_cast<int>(str_result.error()), ")"); | |||
| } | |||
|
|
|||
| map.emplace(std::string(key), std::string(str_result.value())); | |||
| std::string_view str; | |||
| if (auto error = std::move(str_result).get(str)) { | |||
| return Status::Invalid("Error getting value for key '", std::string(key), | |||
| "': ", simdjson::error_message(error)); | |||
| } | |||
|
|
|||
| map.emplace(std::string(key), std::string(str)); | |||
There was a problem hiding this comment.
auto field has type simdjson_result<sj::field>. If we first resolve this to a sj::field, we do not need duplicated value checks for the unescaped_key and value.
Also, first asserting the value (L110), then type-checking the value (L98), is cleaner IMO. (It also works the other way around because simdjson provides overloads for many methods on its result type)
Note that for asserting the value and type-checking the value #50653 adds helper methods we might want to reuse
| bool value; | ||
| if (auto error = std::move(bool_result).get(value)) { | ||
| return Status::Invalid("Error getting bool for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } |
There was a problem hiding this comment.
#50653 adds
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
that already returns arrow::Results instead of error codes. This might simplify these type-casts
|
We should rebase on main before we merge this... This broke our CI... |
See #50732 |
### Rationale for this change The simdjson API has a throwing and non-throwing subset. #50672 activated a compiler flag that disables the throwing subset of the API, which broke some CI builds. ### What changes are included in this PR? This changes `json_write_internal.cc` and `from_string.cc` to use the non-throwing simdjson api ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: #50730 Authored-by: Alexander Taepper <alexander.taepper@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bb0cf77. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
This change enables building Arrow with
SIMDJSON_EXCEPTIONS=0by updating the JSON object parser to use simdjson's non-throwing API.What changes are included?
SIMDJSON_EXCEPTIONS=0for the bundled simdjson dependency.simdjson_result::value()inObjectParserwith the non-throwingget()API.Statusvalues on simdjson errors.Are these changes tested?
Yes.
Fixes: #50654
simdjson#50654